Add remaining Genslip parameters to SRFConfig#93
Conversation
…tests Co-authored-by: lispandfound <12835929+lispandfound@users.noreply.github.com>
Testing InstructionsTo test that your defaults are validated by your schema:
There are a couple of tests that use the |
workflow/schemas.py
Outdated
| Literal( | ||
| "roughnessfile", | ||
| description="Path to roughness file (null = no file).", | ||
| ): Or(str, None), |
There was a problem hiding this comment.
Remove this, it should be passed as a command line argument (at a later date). Make sure it is removed from the tests and yaml defaults as well.
There was a problem hiding this comment.
Removed roughnessfile from schemas, realisations, defaults, and tests in commit 5401095. Also removed init_slip_file and read_slip_file which were similarly removed from SRFConfig in earlier commits.
Co-authored-by: Jake Faulkner <jakefaulkn@gmail.com>
…ad_slip_file, rvfrac_slip_sig
…ad_slip_file, rvfrac_slip_sig
tests/test_realisation_to_srf.py
Outdated
| "hyb_corlen_dep_range=1.5", | ||
| "hyb_corlen_fac=2.0", | ||
| "hyb_corlen_flag=0", | ||
| "hyb_corlen_kmodel=schemas.KModel.SUZUKI", |
There was a problem hiding this comment.
deserialise this like so
elif isinstance(value, Enum):
return value.value
in realisation-to-srf
There was a problem hiding this comment.
Added elif isinstance(value, Enum): serialised_value = str(value.value) to _build_genslip_command in commit 5401095. Enum values now serialize to their integer values (e.g., kmodel=2, hyb_corlen_kmodel=5).
tests/test_realisation.py
Outdated
| "tsfac_slope": -0.5, | ||
| "circular_average": False, | ||
| "kmodel": 2.0, | ||
| "kord": 4.0, |
There was a problem hiding this comment.
This should be an integer, as discussed the test is not smart enough to know this and I will update later. But for now, change this to
| "kord": 4.0, | |
| "kord": 4, |
There was a problem hiding this comment.
Fixed in commit 5401095 - "kmodel": 2 (int, from IntEnum serialization).
Co-authored-by: lispandfound <12835929+lispandfound@users.noreply.github.com>
lispandfound
left a comment
There was a problem hiding this comment.
I am happy to merge this once ruff is happy, the tests pass and Cesar/Felipe has had a chance to once over all the defaults and check they are reasonable or match the genslip defaults.
There was a problem hiding this comment.
Pull request overview
This PR expands the SRF/genslip configuration surface area so the workflow can drive (nearly) the full genslip v5.6.2 parameter set from realisation configs, with defaults aligned to genslip’s source defaults.
Changes:
- Added many missing genslip SRF parameters (with defaults) under
srf:in the root defaults YAML, and addedrvfrac_slip_sigtorupture_velocity. - Extended
SRF_SCHEMAwith validators for the new SRF fields and introducedKModelas anIntEnum. - Extended
SRFConfig/RuptureVelocityand updated genslip command building to correctly serialiseEnumvalues; updated tests accordingly.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
workflow/scripts/realisation_to_srf.py |
Serialises Enum/IntEnum SRFConfig values into genslip CLI args; adds rvfrac_slip_sig handling. |
workflow/schemas.py |
Adds KModel enum and expands SRF/RV schema with validators for many new genslip fields. |
workflow/realisations.py |
Extends SRFConfig and RuptureVelocity dataclasses with typed fields for the new parameters. |
workflow/default_parameters/root/defaults.yaml |
Adds the expanded SRF defaults and rvfrac_slip_sig default. |
tests/test_realisation_to_srf.py |
Updates genslip command expectations and config construction to include new fields. |
tests/test_realisation.py |
Updates SRF config example expectations to include the new fields/types. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Literal( | ||
| "rvfrac_slip_sig", | ||
| description="Rupture velocity fraction slip sigma (null = disabled)."): Or( | ||
| NUMBER, None |
There was a problem hiding this comment.
rvfrac_slip_sig is a sigma, but the schema allows any NUMBER (including negatives) when not null. Since the sentinel disable value is represented as null/None, it would be safer to validate the numeric case as non-negative (e.g., Or(And(NUMBER, _is_non_negative), None)) to prevent passing invalid negative sigmas through to genslip.
| NUMBER, None | |
| And(NUMBER, _is_non_negative), None |
| stype: str | None | ||
| """Slip time function type for genslip (None = use genslip default).""" |
There was a problem hiding this comment.
stype is annotated as str | None, but the schema validates it with Use(Stype), which will produce a schemas.Stype (a StrEnum). For consistency (and clearer typing for callers), consider changing the field type to schemas.Stype | None (and updating the docstring accordingly).
| stype: str | None | |
| """Slip time function type for genslip (None = use genslip default).""" | |
| stype: schemas.Stype | None | |
| """Slip time function type for genslip as a schemas.Stype (None = use genslip default).""" |
| hyb_corlen_flag: bool | ||
| """Enable hybrid correlation length model.""" | ||
| hyb_corlen_kmodel: schemas.KModel | ||
| """Hybrid correlation length k-model index. Stored as float for schema compatibility.""" |
There was a problem hiding this comment.
The hyb_corlen_kmodel docstring says it's "Stored as float for schema compatibility", but the field is now typed as schemas.KModel. Updating the docstring to reflect the enum (and/or the schema coercion) would avoid confusion for future readers.
| """Hybrid correlation length k-model index. Stored as float for schema compatibility.""" | |
| """Hybrid correlation length k-model (schemas.KModel), parsed/coerced from the schema value.""" |
| Literal( | ||
| "hyb_corlen_kmodel", | ||
| description="Hybrid correlation length k-model index.", | ||
| ): And(NUMBER, _is_positive), |
There was a problem hiding this comment.
hyb_corlen_kmodel is typed as KModel in SRFConfig, but the schema currently validates it as And(NUMBER, _is_positive). This both (a) rejects the valid KModel.INPUT_CORNERS = -1 value and (b) allows non-integer floats. Consider validating/coercing with Use(KModel) (or an explicit membership check against KModel) so YAML integers are converted to the enum and invalid numeric values are rejected.
| ): And(NUMBER, _is_positive), | |
| ): Use(KModel), |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The codebase exposed only a small subset of genslip v5.6.2 parameters. This adds the full parameter set with correct defaults extracted from the genslip source.
Changes
defaults.yaml: ~90 new parameters added undersrf:, grouped by category (rise time, hybrid correlation length, rupture velocity, tapering, time shift factors, kinematic models, roughness/wavelength, miscellaneous, aseismic).rvfrac_slip_sigadded torupture_velocitysection.schemas.py:SRF_SCHEMAextended with validators for all new fields. AddedKModelIntEnumfor kinematic model index.rvfrac_slip_sigmoved toRUPTURE_VELOCITY_SCHEMA. Fields deferred to future CLI arguments (roughnessfile,init_slip_file,read_slip_file) excluded.realisations.py:SRFConfigdataclass extended with typed fields.kmodel/hyb_corlen_kmodeluse theKModelenum;kordisint.rvfrac_slip_sigmoved toRuptureVelocity.realisation_to_srf.py:_build_genslip_commandhandlesEnumvalues by serialising to their integer value.test_srf_config_exampleandtest_build_genslip_command_static_argsupdated to cover all new fields with correct types.Design decisions
0/1flagsbool0/1in command-1,-999,1e15sentinelsNone_build_genslip_commandkmodel,hyb_corlen_kmodel)KModel(IntEnum)kord)intAnd(int, _is_positive)str | NoneNonervfrac_slip_sig)float | NoneinRuptureVelocitySRFConfigfor future HF syncOriginal prompt
This section details on the original issue you should resolve
<issue_title>Add remaining Genslip parameters into the codebase</issue_title>
<issue_description>Currently we expose a limited subset of genslip parameters in the code base, but we could do to add the rest. Here are the default values in YAML format that I have extracted from
genslip_v5.6.2.c:The tasks to complete this feature would be as follows:
defaults.yamlfile: here.realisation-to-srfwith an input realisation. You could probably steal one from Cesar or Ayushi and update it using the migration tool on your new branch. The genslip command is built automatically from the realisation config in _build_genslip_command.</issue_description>
<agent_instructions>You may use these defaults for named constants
#define DEFAULT_DHYPO_FRAC 0.75 /* hypo at 0.75 down-dip width /
#define DEFAULT_SHYPO_STEP 20.0 / hypo spacing at 20 km along strike /
#define DEFAULT_SHYPO_MIN_OFF 1.0 / hypos start at 1.0 km along strike /
#define DEFAULT_SLIPS_TO_HYPOS 2 / #slip models = 2 times #hypos */
#define DEFAULT_DT 0.1
#define NTMAX 10000
#define SOMERVILLE_FLAG 1
#define MAI_FLAG 2
#define INPUT_CORNERS_FLAG -1
#define MINSLIP 1.0e-02
#define DEFAULT_KMODEL 2
#define DEFAULT_FLIP_AT_SURFACE 1
#define DEFAULT_STRETCH_KCORNER 0
#define DEFAULT_CIRCULAR_AVERAGE 0
#define DEFAULT_MODIFIED_CORNERS 0
#define DEFAULT_TRUNCATE_ZERO_SLIP 1
#define DEFAULT_RAND_RAKE_DEGS 60.0
#define DEFAULT_SLIP_SIGMA 0.85
#define DEFAULT_TSFAC -0.5
#define DEFAULT_TSFAC_COEF 1.8
#define DEFAULT_TSFAC_FACTOR 1
#define DEFAULT_RT_SCALEFAC 1
#define DEFAULT_VR_TO_VS_FRAC 0.8 /* vrup = 0.8 times local Vs /
#define DEFAULT_SHAL_VRUP_FRAC 0.7 / shallow_vrup...
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.